fix(server): properly parse multiple gas config #18537
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julienrbrt
added
backport/v0.47.x
WalkthroughIt looks like you want to generate a summary and a table of changes based on the provided information. Here's the updated content based on your instructions: WalkthroughThe changes involve bug fixes and refactoring in a Go application related to blockchain or cryptocurrency. A panic issue with gas configuration, a pubkey error in transaction simulation, and a block finalization call issue were fixed. A data race was also resolved. The server's Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- tools/confix/data/v0.47-app.toml
- tools/confix/data/v0.50-app.toml
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- server/README.md (1 hunks)
- server/config/config.go (2 hunks)
- server/config/config_test.go (1 hunks)
- server/config/toml.go (1 hunks)
- simapp/simd/cmd/commands.go (1 hunks)
Files skipped from review due to trivial changes (1)
- simapp/simd/cmd/commands.go
Additional comments: 5
CHANGELOG.md (1)
- 70-70:
The changelog entries are well-documented and include references to the relevant pull requests, which is good for traceability.server/config/toml.go (1)
- 18-22: The change in the configuration file format from using a semicolon to a comma as the separator for specifying minimum gas prices is correct and aligns with the standard format for multiple values in TOML files. This change should resolve the parsing issue reported in issue [Bug]: Incorrect example to set multiple denoms for
mininum-gas-prices#18536.server/config/config.go (2)
3-8: The removal of the "strings" package import is appropriate given that the updated code does not use it.
201-215: The
GetMinGasPricesfunction now correctly usessdk.ParseDecCoinsto parse the minimum gas prices. The error handling with a panic is a significant change; ensure that this behavior is documented and that callers are aware that this function can panic.server/README.md (1)
- 74-84: - The refactoring of the
newAppfunction to acceptservertypes.AppOptionsinstead ofserver.AppOptionsis a significant change. Ensure that all references to this function are updated accordingly.
- The introduction of
baseappOptionsas variadic arguments tosimapp.NewSimAppis a good practice for flexibility and maintainability.- Verify that the
DefaultBaseappOptionsfunction correctly extracts the necessary options fromappOptsand that thesimapp.NewSimAppfunction is compatible with the variadicbaseappOptionsparameter.- Ensure that the documentation reflects the correct usage of the new function signature and the rationale behind the changes.
Comment on lines
+21
to
33
| func TestGetAndSetMinimumGas(t *testing.T) { | ||
| cfg := DefaultConfig() | ||
| cfg.SetMinGasPrices(sdk.DecCoins{sdk.NewInt64DecCoin("foo", 5)}) | ||
|
|
||
| input := sdk.DecCoins{sdk.NewInt64DecCoin("foo", 5)} | ||
| cfg.SetMinGasPrices(input) | ||
| require.Equal(t, "5.000000000000000000foo", cfg.MinGasPrices) | ||
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | ||
|
|
||
| input = sdk.DecCoins{sdk.NewInt64DecCoin("bar", 1), sdk.NewInt64DecCoin("foo", 5)} | ||
| cfg.SetMinGasPrices(input) | ||
| require.Equal(t, "1.000000000000000000bar,5.000000000000000000foo", cfg.MinGasPrices) | ||
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | ||
| } |
There was a problem hiding this comment.
The test cases in TestGetAndSetMinimumGas have been updated to include scenarios with multiple minimum gas prices. However, the assertions for cfg.MinGasPrices are directly comparing the string representation of DecCoins which might not be robust if the string representation changes in the future. It would be better to compare DecCoins directly.
- require.Equal(t, "5.000000000000000000foo", cfg.MinGasPrices)
+ require.EqualValues(t, input.String(), cfg.MinGasPrices)
- require.Equal(t, "1.000000000000000000bar,5.000000000000000000foo", cfg.MinGasPrices)
+ require.EqualValues(t, input.String(), cfg.MinGasPrices)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| func TestGetAndSetMinimumGas(t *testing.T) { | |
| cfg := DefaultConfig() | |
| cfg.SetMinGasPrices(sdk.DecCoins{sdk.NewInt64DecCoin("foo", 5)}) | |
| input := sdk.DecCoins{sdk.NewInt64DecCoin("foo", 5)} | |
| cfg.SetMinGasPrices(input) | |
| require.Equal(t, "5.000000000000000000foo", cfg.MinGasPrices) | |
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | |
| input = sdk.DecCoins{sdk.NewInt64DecCoin("bar", 1), sdk.NewInt64DecCoin("foo", 5)} | |
| cfg.SetMinGasPrices(input) | |
| require.Equal(t, "1.000000000000000000bar,5.000000000000000000foo", cfg.MinGasPrices) | |
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | |
| } | |
| func TestGetAndSetMinimumGas(t *testing.T) { | |
| cfg := DefaultConfig() | |
| input := sdk.DecCoins{sdk.NewInt64DecCoin("foo", 5)} | |
| cfg.SetMinGasPrices(input) | |
| require.EqualValues(t, input.String(), cfg.MinGasPrices) | |
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | |
| input = sdk.DecCoins{sdk.NewInt64DecCoin("bar", 1), sdk.NewInt64DecCoin("foo", 5)} | |
| cfg.SetMinGasPrices(input) | |
| require.EqualValues(t, input.String(), cfg.MinGasPrices) | |
| require.EqualValues(t, cfg.GetMinGasPrices(), input) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 70-70:
The changelog entries are well-documented and include references to the relevant pull requests, which is good for traceability.
tac0turtle
approved these changes
Nov 22, 2023
likhita-809
approved these changes
Nov 22, 2023
mergify bot
pushed a commit
that referenced
this pull request
Nov 22, 2023
(cherry picked from commit 875a71c)
mergify bot
pushed a commit
that referenced
this pull request
Nov 22, 2023
(cherry picked from commit 875a71c) # Conflicts: # simapp/simd/cmd/commands.go # tools/confix/data/v0.47-app.toml # tools/confix/data/v0.50-app.toml
This was referenced Nov 22, 2023
julienrbrt
added a commit
that referenced
this pull request
Nov 22, 2023
…8543) Co-authored-by: Julien Robert <[email protected]>
julienrbrt
added a commit
that referenced
this pull request
Nov 22, 2023
…8544) Co-authored-by: Julien Robert <[email protected]>
rkollar
pushed a commit
to sagaxyz/cosmos-sdk
that referenced
this pull request
Feb 26, 2024
… (cosmos#18544) Co-authored-by: Julien Robert <[email protected]>
12 tasks
emidev98
pushed a commit
to terra-money/cosmos-sdk
that referenced
this pull request
Mar 21, 2024
… (cosmos#18544) Co-authored-by: Julien Robert <[email protected]>
5 tasks
jaeseung-bae
added a commit
to Finschia/finschia-sdk
that referenced
this pull request
Apr 26, 2024
…s-sdk#18537) (#1337) * fix(server): properly parse multiple gas config(backport cosmos-sdk#18537) * chore: update changelog * chore: add missing PR link
mergify bot
pushed a commit
to Finschia/finschia-sdk
that referenced
this pull request
Apr 26, 2024
…s-sdk#18537) (#1337) * fix(server): properly parse multiple gas config(backport cosmos-sdk#18537) * chore: update changelog * chore: add missing PR link (cherry picked from commit 7849929)
5 tasks
mergify bot
pushed a commit
to Finschia/finschia-sdk
that referenced
this pull request
Apr 26, 2024
…s-sdk#18537) (#1337) * fix(server): properly parse multiple gas config(backport cosmos-sdk#18537) * chore: update changelog * chore: add missing PR link (cherry picked from commit 7849929) # Conflicts: # CHANGELOG.md
5 tasks
jaeseung-bae
added a commit
to Finschia/finschia-sdk
that referenced
this pull request
May 6, 2024
…s-sdk#18537) (#1337) (#1343) * fix(server): properly parse multiple gas config(backport cosmos-sdk#18537) * chore: update changelog * chore: add missing PR link (cherry picked from commit 7849929) Co-authored-by: jaeseung-bae <[email protected]>
0Tech
added a commit
to Finschia/finschia-sdk
that referenced
this pull request
May 24, 2024
…s-sdk#18537) (backport #1337) (#1344) * fix(server): properly parse multiple gas config(backport cosmos/cosmos-sdk#18537) (#1337) * fix(server): properly parse multiple gas config(backport cosmos-sdk#18537) * chore: update changelog * chore: add missing PR link (cherry picked from commit 7849929) # Conflicts: # CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: jaeseung-bae <[email protected]> Co-authored-by: Youngtaek Yoon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes: #18536
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Bug Fixes
FinalizeBlockcalls are now correctly passed to ABCIListeners.BaseApp.getContextas identified by e2e tests.Refactor
newAppfunction to accept new types of application options and streamlined the passing of arguments.Documentation
Style